Skip to content

Add error message - Comments.scala:128 #1621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 25, 2016

Conversation

iamthiago
Copy link
Contributor

This commit adds the semantic object for the proper definition not found error.
It is part of the #1589

@iamthiago
Copy link
Contributor Author

Ok, this is my first PR over here, so go easy with me :)
I have some doubts related to the explanation of this error. I particularly believe we should have something there, but I have no idea. I would appreciate any help.

@iamthiago iamthiago force-pushed the feature/error-messages branch from 52a60db to 3b0b5a8 Compare October 23, 2016 23:32
import Symbols._
import Contexts._
import Flags.EmptyFlags
import dotty.tools.dotc.reporting.diagnostic.messages.ProperDefinitionNotFound
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a lot of these changes can be left out, basically the only addition here should be something like:

// you should be able to omit most of the path and get something like:
import reporting.diagnostic.messages.ProperDefinitionNotFound
// or even just:
import reporting.diagnostic.messages._

whichever floats your goat :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh cool, actually I think intellij tried to optimise it for me, but you know, does not always help :)

@@ -125,7 +129,7 @@ object Comments {
val newName = (tree.name.show + "$" + codePos + "$doc").toTermName
untpd.DefDef(newName, tree.tparams, tree.vparamss, tree.tpt, tree.rhs)
case _ =>
ctx.error("proper definition was not found in `@usecase`", codePos)
ctx.error(ProperDefinitionNotFound(), codePos)
Copy link
Contributor

@felixmulder felixmulder Oct 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get a good sense of what's going on here for the error message's explanation there are a couple of things you (or the user) need(s) to know:

  1. What are @usecase?
  2. Why do we care that we're getting an untpd.DefDef here?

val msg = hl"""|A proper definition was not found in ${"@usecase"}"""
val explanation = ""
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usecases are a workaround for ugly signatures that the user of a library does not need to know about. For instance:

trait List[+A] {
  def map[B, That](f: A => B)(implicit bf: CanBuildFrom[List[A], B, That] : That = ???
}

now the users of the list are going "dafuq 😕". Basically the implicit builder will effectively build a collection of the expected type - but the user just assumes that's what happens when you map - and doesn't need to know that.

So if we do this:

/** Map from List[A] => List[B]
  *
  * @usecase def map[B](f: A => B): List[B]
  */
def map[B, That](f: A => B)(implicit bf: CanBuildFrom[List[A], B, That]): That = ???

when building the documentation, the doc tool (which is a sort of compiler) will switch out the definition of map for the one defined in the @usecase. As such, the user reading the docs, won't see the ugly definition and be blissfully unaware :)

Because of this, the error occurs when the user has not written a def but something else (like val map: List[B] or w/e).

So perhaps the explanation can contain some of this information?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great explanation, thanks for your time to do that, I will be working on it and should push a fix later :)

@iamthiago iamthiago force-pushed the feature/error-messages branch 2 times, most recently from 5a9f3e2 to 96284b9 Compare October 24, 2016 20:24
@iamthiago
Copy link
Contributor Author

@felixmulder I have updated the code based on your explanation, take a look and let me know if you agree. Also, there is another PR with approved state with the same number(18), perhaps a merge should occur.

@felixmulder
Copy link
Contributor

You're right, update yours to 19? I'll have a look at your changes first thing in the morning :)

@iamthiago iamthiago force-pushed the feature/error-messages branch from 96284b9 to a6c99dc Compare October 25, 2016 01:53
@iamthiago
Copy link
Contributor Author

iamthiago commented Oct 25, 2016

yeah, I did a rebase with master and now it's 19!
Sorry the delay, but besides timezone, I still have to work 8 hours a day:( but we will get used to it and I will help as much as I can :)

|This way you can hide the ugly definition by providing an easy readable version.
|
|""".stripMargin
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that this explanation still needs some improvement - I feel that it is not clear where you should put the @usecase "thingy". With the current explanation somebody might mistake it for an annotation - or am I in the wrong here?

As I mentioned before - @usecase is added to the docstring of the method. As such perhaps something roughly like:

val explanation = {
  val noUsecase =
    "def map[B, That](f: A => B)(implicit bf: CanBuildFrom[List[A], B, That]): That"

  val usecase =
    """|/** Map from List[A] => List[B]
       |  *
       |  * @usecase def map[B](f: A => B): List[B]
       |  */
       |def map[B, That](f: A => B)(implicit bf: CanBuildFrom[List[A], B, That]): That""".stripMargin

  hl"""|Usecases are only supported for ${"def"}s. They exist because with Scala's
       |advanced type-system, we sometimes end up with seemingly scary signatures.
       |The usage of these methods, however, needs not be - for instance the `map`
       |function
       |
       |${"List(1, 2, 3).map(2 * _) // res: List(2, 4, 6)"}
       |
       |is easy to understand and use - but has a rather bulky signature:
       |
       |$noUsecase
       |
       |to mitigate this and ease the usage of such functions we have the ${"@usecase"}
       |annotation for docstrings. Which can be used like this:
       |
       |$usecase
       |
       |When creating the docs, the signature of the method is substituted by the
       |usecase and the compiler makes sure that it is valid. Because of this, you're
       |only allowed to use ${"def"}s when defining usecases.""".stripMargin
}

could be the basis for your explanation :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done again :) Thank you!

@felixmulder
Copy link
Contributor

Don't worry about the time @thiagoandrade6 - there's no rush :)

@iamthiago iamthiago force-pushed the feature/error-messages branch from a6c99dc to 97b0af7 Compare October 25, 2016 16:34
@iamthiago
Copy link
Contributor Author

iamthiago commented Oct 25, 2016

@felixmulder Take a look in the code whenever you can. Basically I got your message but I have rewritten some parts of that. I believe anyone who got such error, will now easily get rid of that with this very good explanation :)

@felixmulder
Copy link
Contributor

@thiagoandrade6 - thanks for your patience, I'll merge as soon as the CI passes :)

@iamthiago
Copy link
Contributor Author

@felixmulder thanks for make my life easier, helping me to understand more about how the compile works. I have other several questions, but I will read more the code, docs, talks, whatever exist and asking you on demand. For now let's move on to the next PR :)

@felixmulder felixmulder merged commit f57086d into scala:master Oct 25, 2016
@felixmulder
Copy link
Contributor

No worries - for questions not regarding a specific PR, I'll refer you to our gitter-channel :) happy coding!

@iamthiago iamthiago deleted the feature/error-messages branch October 25, 2016 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants